feat: Write roles fingerprints to /var/log/sysroles.jsonl - #178
Conversation
* Extend the sr_fingerprint module to write syslog to /var/log/sysroles.jsonl in addition to writing them to syslog. * Add unit test for sr_fingerprint.py Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 15 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe module now collects role fingerprints, formats syslog key-value records, optionally persists bounded JSONL records, supports check mode, and includes comprehensive unit coverage. ChangesStructured fingerprint logging
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
playbooks/files/library/sr_fingerprint.py (2)
211-256: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider making
run_moduleeasier to unit test.
run_modulebuildsAnsibleModuleinternally, so the write-failure path (fail_jsonon Lines 247-251) and the check-mode preview branch (Lines 230-239) are not exercised bytest_sr_fingerprint.py, only the extracted helper functions are. Accepting an injected module instance (or splitting the check-mode/write logic into a testable helper) would let tests cover thefail_jsonpath, which is the module's main failure mode.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@playbooks/files/library/sr_fingerprint.py` around lines 211 - 256, Refactor run_module so its AnsibleModule dependency can be injected, or extract the check-mode and JSONL write handling into a separately callable helper. Preserve the existing check-mode preview behavior and ensure tests can exercise the _write_jsonl_log failure path through module.fail_json.
84-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the actual return values.
RETURNis an empty placeholder, but the module now returnsfingerprint, and conditionallymessage,jsonl_row, andlog_file(Lines 231-239, 256). Document these fields soansible-docand downstream consumers ofregister:output can see the real return contract.♻️ Suggested RETURN documentation
-RETURN = r""" # """ +RETURN = r""" +fingerprint: + description: The canonical fingerprint record that was logged. + type: dict + returned: always +message: + description: Human-readable preview of the syslog line (check mode only). + type: str + returned: when check mode is enabled +jsonl_row: + description: The JSON line that would be (or was) appended to the log file. + type: str + returned: when write_log_file is true +log_file: + description: Path to the JSONL log file. + type: str + returned: when write_log_file is true +"""🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@playbooks/files/library/sr_fingerprint.py` at line 84, Replace the placeholder RETURN documentation in the sr_fingerprint module with a complete description of the actual return contract: always document fingerprint, and document the conditional message, jsonl_row, and log_file fields, including when each is produced, so ansible-doc and registered results expose these values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@inventory/group_vars/active_roles.yml`:
- Line 12: Update the entry in present_files to reference
tests/unit/test_sr_fingerprint.py instead of the .yml path, matching the actual
added test file and preserving the exact path expected by file-sync automation.
In `@playbooks/files/library/sr_fingerprint.py`:
- Around line 175-188: Preserve the actual distribution values returned by
`_collect_fingerprint_record` and its `managed_node_distro` field. Do not mark
the entire `ansible_facts` input as `no_log=True`; instead, pass only the fields
consumed by `_get_managed_node_distro` as separate parameters while retaining
the existing record-building flow.
---
Nitpick comments:
In `@playbooks/files/library/sr_fingerprint.py`:
- Around line 211-256: Refactor run_module so its AnsibleModule dependency can
be injected, or extract the check-mode and JSONL write handling into a
separately callable helper. Preserve the existing check-mode preview behavior
and ensure tests can exercise the _write_jsonl_log failure path through
module.fail_json.
- Line 84: Replace the placeholder RETURN documentation in the sr_fingerprint
module with a complete description of the actual return contract: always
document fingerprint, and document the conditional message, jsonl_row, and
log_file fields, including when each is produced, so ansible-doc and registered
results expose these values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2eb2986c-d66c-4fdc-9d50-f2c7b1cde1aa
📒 Files selected for processing (3)
inventory/group_vars/active_roles.ymlplaybooks/files/library/sr_fingerprint.pyplaybooks/files/tests/unit/test_sr_fingerprint.py
* Add max_log_lines defaulting to 10000 * Apply CodeRabbit review * Change the need for large ansible_facts to smaller distribution and distribution_version
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
playbooks/files/tests/unit/test_sr_fingerprint.py (2)
251-271: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAvoid the hardcoded
/tmppath to prevent a lint failure.Lines 256 and 269 use the literal
"/tmp/test.jsonl". Static analysis flags this as CWE-377 (Ruff S108, ast-grephardcoded-tmp-file) at the error level. The test never opens this file, becausecheck_mode=Truemakes_handle_fingerprintreturn before the write branch runs. Even so, the literal path still triggers the lint rule and can fail the pipeline.Derive the path with
tempfile.gettempdir()instead of a literal/tmpstring.🔧 Proposed fix
def test_handle_fingerprint_check_mode_with_log_file(self): + log_file = os.path.join(tempfile.gettempdir(), "test.jsonl") module = _FakeModule( { "status": "success", "write_log_file": True, - "log_file": "/tmp/test.jsonl", + "log_file": log_file, "role_name": "systemd", "role_path": "/usr/share/ansible/roles/systemd", "ansible_play_hosts_all": ["host1"], "distribution": "RedHat", "distribution_version": "9.4", }, check_mode=True, ) with self.assertRaises(_ExitJsonException) as ctx: sr_fingerprint._handle_fingerprint(module) result = ctx.exception.kwargs self.assertIn("jsonl_row", result) - self.assertEqual(result["log_file"], "/tmp/test.jsonl") + self.assertEqual(result["log_file"], log_file)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@playbooks/files/tests/unit/test_sr_fingerprint.py` around lines 251 - 271, Replace the hardcoded “/tmp/test.jsonl” values in test_handle_fingerprint_check_mode_with_log_file with a path derived from tempfile.gettempdir(), and use the same derived path for both the fake module input and the assertion. Preserve the existing test behavior and add or reuse the tempfile import as needed.Source: Linters/SAST tools
273-291: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the fingerprint write-failure test independent from non-root execution.
This test uses
/nonexistent/deep/pathand expects directory creation or file opening to fail. As root, the write can succeed, so the expected failure path is not covered. Mock the failure, such asbuiltins.openor_ensure_parent_dir, to make the assertion deterministic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@playbooks/files/tests/unit/test_sr_fingerprint.py` around lines 273 - 291, Update test_handle_fingerprint_write_failure_calls_fail_json to mock the file-writing failure deterministically, using builtins.open or _ensure_parent_dir, instead of relying on the /nonexistent/deep/path filesystem behavior. Preserve the existing _FailJsonException assertion and failure-message validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@playbooks/files/library/sr_fingerprint.py`:
- Around line 186-200: Update _write_jsonl_log and _trim_log_file to hold a
cross-process lock across the append and optional trim operations. Perform
trimming by writing retained lines to a temporary file in the log’s directory,
flushing and closing it, then atomically replacing the original log; ensure
concurrent writers cannot overwrite rows added after their snapshot.
- Line 299: Validate max_log_lines before fingerprint handling so negative
values are rejected, while preserving 0 as the documented value that disables
trimming and positive values as valid limits.
---
Nitpick comments:
In `@playbooks/files/tests/unit/test_sr_fingerprint.py`:
- Around line 251-271: Replace the hardcoded “/tmp/test.jsonl” values in
test_handle_fingerprint_check_mode_with_log_file with a path derived from
tempfile.gettempdir(), and use the same derived path for both the fake module
input and the assertion. Preserve the existing test behavior and add or reuse
the tempfile import as needed.
- Around line 273-291: Update
test_handle_fingerprint_write_failure_calls_fail_json to mock the file-writing
failure deterministically, using builtins.open or _ensure_parent_dir, instead of
relying on the /nonexistent/deep/path filesystem behavior. Preserve the existing
_FailJsonException assertion and failure-message validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 44120bfa-fa79-4e4c-ab39-285f4c54179c
📒 Files selected for processing (4)
inventory/group_vars/active_roles.ymlplaybooks/files/library/sr_fingerprint.pyplaybooks/files/tests/unit/sr_fingerprint.pyplaybooks/files/tests/unit/test_sr_fingerprint.py
🚧 Files skipped from review as they are similar to previous changes (1)
- inventory/group_vars/active_roles.yml
|
I think using a max size in bytes rather than a max number of records might be easier to implement: |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@playbooks/files/library/sr_fingerprint.py`:
- Around line 230-231: Define and enforce an explicit oversized-record policy in
the JSONL write flow around the max_size check and _trim_log_file: either raise
a clear module error when a serialized row exceeds max_log_size, or
intentionally allow that single row without trimming and document the exception.
Ensure missing log files do not trigger _trim_log_file with a negative or
invalid target, preserve consistent behavior for existing files, and add a
regression test covering a missing log file whose limit is smaller than one row.
- Line 195: Rename the ambiguous loop variable l to line in the while condition
and any related expressions within the surrounding logic, preserving the
existing behavior of the size calculation and line processing.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e93759fa-f9c4-4a1d-88e6-499762bed81c
📒 Files selected for processing (2)
playbooks/files/library/sr_fingerprint.pyplaybooks/files/tests/unit/test_sr_fingerprint.py
| if max_size > 0 and cur_size + len(new_line) > max_size: | ||
| _trim_log_file(log_file, max_size - len(new_line)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Define behavior for a record larger than max_log_size.
If one JSONL row exceeds max_log_size and the log file does not exist, this path calls _trim_log_file, which attempts to read the missing file. The module then reports a generic write failure. If the file already exists, it removes old rows and still appends a row larger than the documented maximum.
Define one oversized-record policy before trimming. Reject an impossible limit with a clear module error, or allow one oversized row without trimming and document that exception. Add a regression test for a missing log file with a limit smaller than one row.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 231-231: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(log_file, "a")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@playbooks/files/library/sr_fingerprint.py` around lines 230 - 231, Define and
enforce an explicit oversized-record policy in the JSONL write flow around the
max_size check and _trim_log_file: either raise a clear module error when a
serialized row exceeds max_log_size, or intentionally allow that single row
without trimming and document the exception. Ensure missing log files do not
trigger _trim_log_file with a negative or invalid target, preserve consistent
behavior for existing files, and add a regression test covering a missing log
file whose limit is smaller than one row.
|
|
||
|
|
||
| def _handle_fingerprint(module): | ||
| max_log_size = module.params.get("max_log_size", 0) |
There was a problem hiding this comment.
note that every module param with a required=True or default will always exist in module.params - so module.params["max_log_size"] is safe to use - plus you don't have another default value which might cause confusion
| except OSError: | ||
| cur_size = 0 | ||
| if max_size > 0 and cur_size + len(new_line) > max_size and cur_size > 0: | ||
| _trim_log_file(log_file, max_size - len(new_line)) |
There was a problem hiding this comment.
| _trim_log_file(log_file, max_size - len(new_line)) | |
| _trim_log_file(log_file, len(new_line)) |
| return json.dumps(record, separators=(",", ":"), sort_keys=False) | ||
|
|
||
|
|
||
| def _trim_log_file(log_file, target_size): |
There was a problem hiding this comment.
| def _trim_log_file(log_file, target_size): | |
| def _trim_log_file(log_file, size_needed): |
|
|
||
|
|
||
| def _trim_log_file(log_file, target_size): | ||
| """Remove oldest records until the file fits in target_size bytes.""" |
There was a problem hiding this comment.
| """Remove oldest records until the file fits in target_size bytes.""" | |
| """Remove oldest records until the file can accommodate size_needed bytes.""" |
| """Remove oldest records until the file fits in target_size bytes.""" | ||
| with open(log_file, "r") as log_fd: | ||
| lines = log_fd.readlines() | ||
| while lines and sum(len(line) for line in lines) > target_size: |
There was a problem hiding this comment.
| while lines and sum(len(line) for line in lines) > target_size: | |
| size_removed = 0 | |
| while lines and size_removed < size_needed: | |
| size_removed += len(lines.pop(0)) |
that way you don't need to recalculate the new size every time, and get the length of every line every time.
2 participants
Extend the sr_fingerprint module to write syslog to /var/log/sysroles.jsonl in addition to writing them to syslog.
Add unit test for sr_fingerprint.py
Summary by CodeRabbit